Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: update the accounting of partial batch mutations #2149

Merged
merged 32 commits into from
Mar 27, 2024
Merged

fix: update the accounting of partial batch mutations #2149

merged 32 commits into from
Mar 27, 2024

Conversation

ron-gal
Copy link
Contributor

@ron-gal ron-gal commented Mar 6, 2024 •
edited
Loading

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
  • Rollback plan is reviewed and LGTMed
  • All new data plane features have a completed end to end testing plan

Fixes #1494 ☕️

If you write sample code, please follow the samples format.

@product-auto-label product-auto-label bot added size: l Pull request size is large. api: bigtable Issues related to the googleapis/java-bigtable API. labels Mar 6, 2024
@ron-gal ron-gal changed the title Fixed retrying of partial errors bug: Fixed retrying of partial errors Mar 12, 2024
@ron-gal ron-gal marked this pull request as ready for review March 12, 2024 19:48
@ron-gal ron-gal requested review from a team as code owners March 12, 2024 19:48
igorbernstein2
igorbernstein2 requested changes Mar 13, 2024
View reviewed changes
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java Outdated Show resolved Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java Outdated Show resolved Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java Outdated Show resolved Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java Outdated Show resolved Hide resolved
.../main/java/com/google/cloud/bigtable/data/v2/stub/MutateRowsErrorConverterUnaryCallable.java Show resolved Hide resolved
...ble/src/main/java/com/google/cloud/bigtable/gaxx/retrying/MutateRowsErrorRetryAlgorithm.java Outdated Show resolved Hide resolved
...ble/src/main/java/com/google/cloud/bigtable/gaxx/retrying/MutateRowsErrorRetryAlgorithm.java Outdated Show resolved Hide resolved
...ble/src/main/java/com/google/cloud/bigtable/gaxx/retrying/MutateRowsErrorRetryAlgorithm.java Outdated Show resolved Hide resolved
...-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java Show resolved Hide resolved
...bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java Outdated Show resolved Hide resolved
@ron-gal ron-gal requested a review from igorbernstein2 March 14, 2024 02:01
igorbernstein2
igorbernstein2 requested changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a bunch of comments. But more broadly the main piece thats missing is tests for the 2 root problems:

  1. that the error messages for partial failures does not include anything about INTERNAL errors
  2. that the metrics for partial failures count as OK

...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java Outdated Show resolved Hide resolved
...src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptResult.java Outdated Show resolved Hide resolved
...src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptResult.java Outdated
import java.util.ArrayList;
import java.util.List;

public class MutateRowsAttemptResult {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about AutoValue?

.../main/java/com/google/cloud/bigtable/gaxx/retrying/MutateRowsPartialErrorRetryAlgorithm.java Outdated Show resolved Hide resolved
.../main/java/com/google/cloud/bigtable/gaxx/retrying/MutateRowsPartialErrorRetryAlgorithm.java Outdated Show resolved Hide resolved
...java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsBatchingDescriptorTest.java Outdated Show resolved Hide resolved
...java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsBatchingDescriptorTest.java Outdated Show resolved Hide resolved
...google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsErrorConverterUnaryCallableTest.java Outdated Show resolved Hide resolved
...google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsErrorConverterUnaryCallableTest.java Outdated Show resolved Hide resolved
...google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsErrorConverterUnaryCallableTest.java Show resolved Hide resolved
@igorbernstein2 igorbernstein2 changed the title bug: Fixed retrying of partial errors fix: update the accounting of partial batch mutations Mar 19, 2024
@igorbernstein2 igorbernstein2 marked this pull request as draft March 19, 2024 15:54
@igorbernstein2 igorbernstein2 marked this pull request as ready for review March 27, 2024 02:58
@igorbernstein2
Copy link
Contributor

Can you add an exception to the clirr config for
7005: com.google.cloud.bigtable.data.v2.stub.mutaterows.MutateRowsBatchingDescriptor: Parameter 1 of 'public void splitResponse(java.lang.Void, java.util.List)' has changed its type to com.google.cloud.bigtable.data.v2.stub.mutaterows.MutateRowsAttemptResult

@igorbernstein2 igorbernstein2 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 27, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 27, 2024
@ron-gal ron-gal requested a review from a team as a code owner March 27, 2024 03:02
@igorbernstein2 igorbernstein2 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 27, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 27, 2024
@igorbernstein2 igorbernstein2 added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels Mar 27, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 27, 2024
@ron-gal
Copy link
Contributor Author

ron-gal commented Mar 27, 2024

/gcbrun

@chingor13 chingor13 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 27, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 27, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit 4158094 into googleapis:main Mar 27, 2024
21 of 22 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Mar 27, 2024
@release-please release-please bot mentioned this pull request Mar 26, 2024
Merged
@ron-gal ron-gal deleted the retry branch March 27, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

Assignees
No one assigned
Labels
api: bigtable Issues related to the googleapis/java-bigtable API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Batcher error accounting is incorrect when the RPC fails
3 participants
@ron-gal @igorbernstein2 @chingor13